fix: Make conversion from FileDecryptionProperties to ConfigFileDecryptionProperties fallible#21603
fix: Make conversion from FileDecryptionProperties to ConfigFileDecryptionProperties fallible#21603adamreeve wants to merge 5 commits intoapache:mainfrom
Conversation
github-actions
bot
added
core
adamreeve
force-pushed
the
fallible-decrypt-config-convert
branch
from
8cc9c06 to
6d380a1
Compare
| type Error = DataFusionError; | ||
|
|
||
| fn try_from(f: &Arc<FileDecryptionProperties>) -> Result<Self> { | ||
| let footer_key = f.footer_key(None).map_err(|e| { |
There was a problem hiding this comment.
This still depends on footer_key(None) failing. A key retriever can return a footer key even when it still cannot represent the full decryption config. In that case thisconversion can still succeed, but column_keys() is empty and we silently lose the column decryption info. Can we reject all key-retriever-based FileDecryptionProperties directly and add a test for that case?
There was a problem hiding this comment.
Yes, that's not currently possible with the public API of FileDecryptionProperties but I can follow up and change that too. I think it would still make sense to make this current change and then improve this later once arrow-rs allows it.
There was a problem hiding this comment.
Sure, Please open a follow-up issue and link it here so that it is tracked. Also handle the upgrade note.
There was a problem hiding this comment.
I've opened a follow-up Datafusion issue at #21634 and an arrow-rs issue at apache/arrow-rs#9721, and will implement the arrow-rs change soon.
| #[cfg(feature = "parquet_encryption")] | ||
| impl From<&Arc<FileDecryptionProperties>> for ConfigFileDecryptionProperties { | ||
| fn from(f: &Arc<FileDecryptionProperties>) -> Self { | ||
| impl TryFrom<&Arc<FileDecryptionProperties>> for ConfigFileDecryptionProperties { |
There was a problem hiding this comment.
This changes a public API from From to TryFrom, so downstream code using (&decrypt).into() or ConfigFileDecryptionProperties::from(&decrypt) will stop compiling. Can we add an upgrade note for this change.
There was a problem hiding this comment.
Good point, I've added a note to the 54.0.0 upgrade guide now.
github-actions
bot
added
the
documentation
adamreeve
force-pushed
the
fallible-decrypt-config-convert
branch
from
d3f2020 to
dc8ad1a
Compare
adamreeve
force-pushed
the
fallible-decrypt-config-convert
branch
from
dc8ad1a to
704d0f1
Compare
|
|
||
| ### Conversion from `FileDecryptionProperties` to `ConfigFileDecryptionProperties` is now fallible | ||
|
|
||
| Previously, `datafusion_common::config::ConfigFileDecryptionProperties` |
There was a problem hiding this comment.
I think FileDecryptionProperties::into is not the old call shape here. Please say (&decrypt).into() becomes (&decrypt).try_into(), or ConfigFileDecryptionProperties::from(...) becomes try_from(...), so the upgrade note matches actual usage.
There was a problem hiding this comment.
Sure, I just wanted to demonstrate the types involved as (&decrypt).into() alone is quite ambiguous. I've fleshed this out more and added before and after examples to make this extra clear.
2 participants
Which issue does this PR close?
Rationale for this change
Fail quickly with a helpful error if we're unable to represent a
FileDecryptionPropertiesinstance asConfigFileDecryptionPropertiesWhat changes are included in this PR?
From<&Arc<FileDecryptionProperties>>forConfigFileDecryptionPropertiestoTryFrom.FileDecryptionPropertieswith empty metadataAre these changes tested?
Yes I've added a new unit test.
I also tested this with a branch of delta-rs that uses Datafusion with Parquet encryption, and this required only minor changes to tests and examples: corwinjoy/delta-rs@file_format_options_squashed...adamreeve:delta-rs:test-datafusion-change
Are there any user-facing changes?
Yes, this is a breaking API change.